-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Change core::iter::Fuse
's Default
impl to do what its docs say it does
#140985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
core::iter::Fuse
's Default
impl to do what it's docs say it doescore::iter::Fuse
's Default
impl to do what its docs say it does
Either way (fix the impl or fix the docs) this should probably gain a test since it’s extremely non-obvious from the code alone. |
And maybe change the code to either |
I doubt this will have much of a noticeable impact but we may as well try to measure it. @bors try |
Change `core::iter::Fuse`'s `Default` impl to do what its docs say it does The [docs on `impl<I: Default> Default for core::iter::Fuse<I>`](https://doc.rust-lang.org/nightly/std/iter/struct.Fuse.html#impl-Default-for-Fuse%3CI%3E) say (as the `I: Default` bound implies) that `Fuse::<I>::default` "Creates a `Fuse` iterator from the default value of `I`". However, the implementation creates a `Fuse` with `Fuse { iter: Default::default() }`, and since the `iter` field is an `Option<I>`, this is actually `Fuse { iter: None }`, not `Fuse { iter: Some(I::default()) }`, so `Fuse::<I>::default()` always returns an empty iterator, even if `I::default()` would not be empty. This PR changes `Fuse`'s `Default` implementation to match the documentation. This will be a behavior change for anyone currently using `Fuse::<I>::default()` where `I::default()` is not an empty iterator[^1], as `Fuse::<I>::default()` will now also not be an empty iterator. (Alternately, the docs could be updated to reflect what the current implementation actually does, i.e. returns an always-exhausted iterator that never yields any items (even if `I::default()` would have yielded items). With this option, the `I: Default` bound could also be removed to reflect that no `I` is ever created.) [Current behavior example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=a1e0adc4badca3dc11bfb70a99213249) (maybe an example like this should be added to the docs either way?) This PR changes publicly observable behavior, so I think requires at least a T-libs-api FCP? r? libs-api cc rust-lang#140961 `impl<I: Default> Default for Fuse<I>` was added in 1.70.0 (rust-lang#99929), and it's docs and behavior do not appear to have changed since (`Fuse`'s `iter` field has been an `Option` since before the impl was added). [^1]: IIUC it is a "de facto" guideline for the stdlib that an iterator type's `default()` should be empty (and for iterators where that would not make sense, they should not implement `Default`): cc rust-lang/libs-team#77 (comment) , so for stdlib iterators, I don't think this would change anything. However, if a user has a custom `Iterator` type `I`, *and* they are using `Fuse<I>`, *and* they call `Fuse::<I>::default()`, this may change the behavior of their code.
☀️ Try build successful - checks-actions |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@craterbot run mode=build-and-test |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
The docs on
impl<I: Default> Default for core::iter::Fuse<I>
say (as theI: Default
bound implies) thatFuse::<I>::default
"Creates aFuse
iterator from the default value ofI
". However, the implementation creates aFuse
withFuse { iter: Default::default() }
, and since theiter
field is anOption<I>
, this is actuallyFuse { iter: None }
, notFuse { iter: Some(I::default()) }
, soFuse::<I>::default()
always returns an empty iterator, even ifI::default()
would not be empty.This PR changes
Fuse
'sDefault
implementation to match the documentation. This will be a behavior change for anyone currently usingFuse::<I>::default()
whereI::default()
is not an empty iterator1, asFuse::<I>::default()
will now also not be an empty iterator.(Alternately, the docs could be updated to reflect what the current implementation actually does, i.e. returns an always-exhausted iterator that never yields any items (even if
I::default()
would have yielded items). With this option, theI: Default
bound could also be removed to reflect that noI
is ever created.)Current behavior example (maybe an example like this should be added to the docs either way?)
This PR changes publicly observable behavior, so I think requires at least a T-libs-api FCP?
r? libs-api
cc #140961
impl<I: Default> Default for Fuse<I>
was added in 1.70.0 (#99929), and it's docs and behavior do not appear to have changed since (Fuse
'siter
field has been anOption
since before the impl was added).Footnotes
IIUC it is a "de facto" guideline for the stdlib that an iterator type's
default()
should be empty (and for iterators where that would not make sense, they should not implementDefault
): cc https://github.com/rust-lang/libs-team/issues/77#issuecomment-1194681709 , so for stdlib iterators, I don't think this would change anything. However, if a user has a customIterator
typeI
, and they are usingFuse<I>
, and they callFuse::<I>::default()
, this may change the behavior of their code. ↩